Use the proper Request in type hints. #9515

clokep · 2021-03-01T14:30:20Z

Twisted comes with twisted.web.http.Request and twisted.web.server.Request, we were using the former in some situations when we should always be using the latter.

This is work towards #9513, it also pins Twisted for the mypy check in the meantime, so that we can continue working in parallel to this.

This reduces the mypy errors from 196 to 129.

clokep · 2021-03-01T14:32:50Z

synapse/handlers/auth.py

@@ -481,7 +481,7 @@ async def check_ui_auth(
            sid = authdict["session"]

        # Convert the URI and method to strings.
-        uri = request.uri.decode("utf-8")
+        uri = request.uri.decode("utf-8")  # type: ignore


The type of request.uri is incorrectly specified in Twisted.

Well it isn't "incorrect", but the placeholder is a string and the value is bytes, which is annoying.

(Maybe make notes on the places where this happens, and submit that issue on twisted/twisted?)

I'm already planning to.

clokep · 2021-03-01T14:33:16Z

synapse/replication/http/membership.py

-        requester = Requester.deserialize(self.store, content["requester"])
-
-        request.requester = requester


Here (and below) the request is never used after this line, so I don't think this is doing anything.

How confident are we? If we're getting request as an input parameter and than mutating it as a side effect, might that have issues elsewhere?

I'm not sure what you're asking. Are you asking if we're confident in this change or if the code before this change might have been masking other bugs or something?

Note that this is the replication API, which I don't think has any concept of requester? The Requester.deserialize doesn't do much except generate an object from a string, so it shouldn't have any real side-effects. Maybe this is here to change logging?

@erikjohnston do you know what the proper thing to do here is?

Yeah, I think is so that the requester is logged, rather than the default {None}. We should probably make it happen automatically, somehow, rather than having it only work for a subset of the replication APIs.

Ah, OK. The weird thing is that that implies this is actually a SynapseRequest then, not a Request. I'll see if I can rework this bit.

8cbc815 reverts removing this.

clokep · 2021-03-01T14:33:42Z

synapse/rest/media/v1/_base.py

        # This allows users to append e.g. /test.png to the URL. Useful for
        # clients that parse the URL to see content type.
-        server_name, media_id = request.postpath[:2]
-
-        if isinstance(server_name, bytes):


This is always bytes so I don't think we need this if-statement.

clokep · 2021-03-01T14:34:05Z

synapse/rest/media/v1/upload_resource.py

@@ -88,8 +90,9 @@ async def _async_render_POST(self, request: Request) -> None:
        # TODO(markjh): parse content-dispostion

        try:
+            content = request.content  # type: IO  # type: ignore


The type of this is undefined right now in Twisted (I think)?

Why the type: ignore after the IO type definition?

It overrides the definition previously calculated.

clokep · 2021-03-01T14:34:43Z

synapse/rest/media/v1/upload_resource.py

@@ -79,7 +79,9 @@ async def _async_render_POST(self, request: Request) -> None:
        headers = request.requestHeaders

        if headers.hasHeader(b"Content-Type"):
-            media_type = headers.getRawHeaders(b"Content-Type")[0].decode("ascii")
+            content_type_headers = headers.getRawHeaders(b"Content-Type")
+            assert content_type_headers  # for mypy


We know that the header will exist due to the above check, but mypy doesn't...

synapse/rest/media/v1/_base.py

@dklimpel

Synapse 1.29.0 (2021-03-08) =========================== Note that synapse now expects an `X-Forwarded-Proto` header when used with a reverse proxy. Please see [UPGRADE.rst](UPGRADE.rst#upgrading-to-v1290) for more details on this change. No significant changes. Synapse 1.29.0rc1 (2021-03-04) ============================== Features -------- - Add rate limiters to cross-user key sharing requests. ([\#8957](matrix-org/synapse#8957)) - Add `order_by` to the admin API `GET /_synapse/admin/v1/users/<user_id>/media`. Contributed by @dklimpel. ([\#8978](matrix-org/synapse#8978)) - Add some configuration settings to make users' profile data more private. ([\#9203](matrix-org/synapse#9203)) - The `no_proxy` and `NO_PROXY` environment variables are now respected in proxied HTTP clients with the lowercase form taking precedence if both are present. Additionally, the lowercase `https_proxy` environment variable is now respected in proxied HTTP clients on top of existing support for the uppercase `HTTPS_PROXY` form and takes precedence if both are present. Contributed by Timothy Leung. ([\#9372](matrix-org/synapse#9372)) - Add a configuration option, `user_directory.prefer_local_users`, which when enabled will make it more likely for users on the same server as you to appear above other users. ([\#9383](matrix-org/synapse#9383), [\#9385](matrix-org/synapse#9385)) - Add support for regenerating thumbnails if they have been deleted but the original image is still stored. ([\#9438](matrix-org/synapse#9438)) - Add support for `X-Forwarded-Proto` header when using a reverse proxy. ([\#9472](matrix-org/synapse#9472), [\#9501](matrix-org/synapse#9501), [\#9512](matrix-org/synapse#9512), [\#9539](matrix-org/synapse#9539)) Bugfixes -------- - Fix a bug where users' pushers were not all deleted when they deactivated their account. ([\#9285](matrix-org/synapse#9285), [\#9516](matrix-org/synapse#9516)) - Fix a bug where a lot of unnecessary presence updates were sent when joining a room. ([\#9402](matrix-org/synapse#9402)) - Fix a bug that caused multiple calls to the experimental `shared_rooms` endpoint to return stale results. ([\#9416](matrix-org/synapse#9416)) - Fix a bug in single sign-on which could cause a "No session cookie found" error. ([\#9436](matrix-org/synapse#9436)) - Fix bug introduced in v1.27.0 where allowing a user to choose their own username when logging in via single sign-on did not work unless an `idp_icon` was defined. ([\#9440](matrix-org/synapse#9440)) - Fix a bug introduced in v1.26.0 where some sequences were not properly configured when running `synapse_port_db`. ([\#9449](matrix-org/synapse#9449)) - Fix deleting pushers when using sharded pushers. ([\#9465](matrix-org/synapse#9465), [\#9466](matrix-org/synapse#9466), [\#9479](matrix-org/synapse#9479), [\#9536](matrix-org/synapse#9536)) - Fix missing startup checks for the consistency of certain PostgreSQL sequences. ([\#9470](matrix-org/synapse#9470)) - Fix a long-standing bug where the media repository could leak file descriptors while previewing media. ([\#9497](matrix-org/synapse#9497)) - Properly purge the event chain cover index when purging history. ([\#9498](matrix-org/synapse#9498)) - Fix missing chain cover index due to a schema delta not being applied correctly. Only affected servers that ran development versions. ([\#9503](matrix-org/synapse#9503)) - Fix a bug introduced in v1.25.0 where `/_synapse/admin/join/` would fail when given a room alias. ([\#9506](matrix-org/synapse#9506)) - Prevent presence background jobs from running when presence is disabled. ([\#9530](matrix-org/synapse#9530)) - Fix rare edge case that caused a background update to fail if the server had rejected an event that had duplicate auth events. ([\#9537](matrix-org/synapse#9537)) Improved Documentation ---------------------- - Update the example systemd config to propagate reloads to individual units. ([\#9463](matrix-org/synapse#9463)) Internal Changes ---------------- - Add documentation and type hints to `parse_duration`. ([\#9432](matrix-org/synapse#9432)) - Remove vestiges of `uploads_path` configuration setting. ([\#9462](matrix-org/synapse#9462)) - Add a comment about systemd-python. ([\#9464](matrix-org/synapse#9464)) - Test that we require validated email for email pushers. ([\#9496](matrix-org/synapse#9496)) - Allow python to generate bytecode for synapse. ([\#9502](matrix-org/synapse#9502)) - Fix incorrect type hints. ([\#9515](matrix-org/synapse#9515), [\#9518](matrix-org/synapse#9518)) - Add type hints to device and event report admin API. ([\#9519](matrix-org/synapse#9519)) - Add type hints to user admin API. ([\#9521](matrix-org/synapse#9521)) - Bump the versions of mypy and mypy-zope used for static type checking. ([\#9529](matrix-org/synapse#9529))

clokep added 7 commits March 1, 2021 09:15

Use the proper Request in handlers.

82df4c4

Use the proper request in media.

ad57f41

Use the proper request in rest/synapse.

bea9314

Use the proper request in rest/client.

d54cf70

Use the proper request in replication.

16b96eb

Pin Twisted when checking mypy for now.

c040e42

Newsfragment

b0e0c72

clokep commented Mar 1, 2021

View reviewed changes

clokep requested a review from a team March 1, 2021 15:07

callahad reviewed Mar 1, 2021

View reviewed changes

synapse/rest/media/v1/_base.py Outdated Show resolved Hide resolved

Clarify comment.

d91327d

clokep requested a review from a team March 1, 2021 15:27

erikjohnston approved these changes Mar 1, 2021

View reviewed changes

Add back the replication requester.

8cbc815

clokep merged commit a0bc9d3 into develop Mar 1, 2021

clokep deleted the clokep/server-request branch March 1, 2021 17:23

This was referenced Mar 15, 2021

Revert requiring a specific version of Twisted for mypy checks. #9618

Merged

10136: t.w.server.Request.postpath is a list of bytes. twisted/twisted#1552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the proper Request in type hints. #9515

Use the proper Request in type hints. #9515

clokep commented Mar 1, 2021 •

edited

Loading

clokep Mar 1, 2021

clokep Mar 1, 2021

ShadowJonathan Mar 1, 2021 •

edited

Loading

clokep Mar 1, 2021

clokep Mar 1, 2021

callahad Mar 1, 2021

clokep Mar 1, 2021

erikjohnston Mar 1, 2021

clokep Mar 1, 2021

clokep Mar 1, 2021

clokep Mar 1, 2021

clokep Mar 1, 2021

ShadowJonathan Mar 1, 2021

clokep Mar 1, 2021

clokep Mar 1, 2021

		requester = Requester.deserialize(self.store, content["requester"])

		request.requester = requester

Use the proper Request in type hints. #9515

Use the proper Request in type hints. #9515

Conversation

clokep commented Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShadowJonathan Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep commented Mar 1, 2021 •

edited

Loading

ShadowJonathan Mar 1, 2021 •

edited

Loading